Skip to content

Fix an issue writer cannot write status type (+)#22

Merged
greedy52 merged 1 commit intorelease/v9.0.0-rc.1from
steve/write_status_type
Dec 14, 2022
Merged

Fix an issue writer cannot write status type (+)#22
greedy52 merged 1 commit intorelease/v9.0.0-rc.1from
steve/write_status_type

Conversation

@greedy52
Copy link
Copy Markdown

@greedy52 greedy52 commented Dec 9, 2022

Related issue:
gravitational/teleport#18776

Cause:
Driver reads +<status> as a golang string so writer will write $<len><status> instead of +<status>

Change:
Define a type alias type StatusString string and write +<status> if interface is StatusString

I think we should also update upstream. Will create a PR to upstream if we all agree.

@greedy52 greedy52 self-assigned this Dec 9, 2022
@greedy52 greedy52 changed the title Fix an issue status type (+) is not read/write properly Fix an issue writer cannot write status type (+) Dec 9, 2022
@greedy52 greedy52 assigned jakule and smallinsky and unassigned jakule and smallinsky Dec 9, 2022
@greedy52 greedy52 requested review from jakule and smallinsky December 9, 2022 21:00
@greedy52 greedy52 marked this pull request as ready for review December 9, 2022 21:00
Copy link
Copy Markdown

@smallinsky smallinsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Copy link
Copy Markdown

@jakule jakule left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@greedy52 Thanks for the fix. LGTM

Do you have any plans to push that fix upstream?

@greedy52
Copy link
Copy Markdown
Author

greedy52 commented Dec 14, 2022

@greedy52 Thanks for the fix. LGTM

Do you have any plans to push that fix upstream?

Yes. I am planning to push it upstream. It's only an internal type so it may not make too much difference for them in general but will open a PR there anyway and see if they will take it.

@greedy52 greedy52 merged commit 8ff76d5 into release/v9.0.0-rc.1 Dec 14, 2022
@greedy52 greedy52 deleted the steve/write_status_type branch December 14, 2022 13:34
jentfoo added a commit that referenced this pull request Mar 7, 2023
greedy52 added a commit that referenced this pull request Mar 8, 2023
* Fix an issue status type (+) is not read/write properly (#22)

* add an option for using StatusString vs string
greedy52 added a commit that referenced this pull request Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants